.claude/skills/ably-new-command/references/patterns.md (1)

403-412: Consider adding language identifiers to example output blocks.

The static analysis tool flags several fenced code blocks without language specifiers (lines 403, 434, 470, 500). While these are example outputs rather than code, adding text or plaintext as the language identifier would satisfy markdownlint and maintain consistency.

✏️ Suggested fix

-```
+```text
 [2024-01-15T10:30:00.000Z]
 ID: msg-123

Apply similar changes to the other flagged blocks at lines 434, 470, and 500.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.claude/skills/ably-new-command/references/patterns.md around lines 403 -
412, Add a language identifier (e.g., "text" or "plaintext") to the fenced
example output blocks so markdownlint stops flagging them; specifically update
the triple-backtick fences around the sample output that begins with
"[2024-01-15T10:30:00.000Z] ID: msg-123 ..." and apply the same change to the
other similar examples in the file (the blocks starting with the outputs flagged
by the reviewer). Ensure you only modify the opening fence to ```text (or
```plaintext) and leave the block contents unchanged.

src/commands/spaces/cursors/set.ts (1)

171-188: Consider using singular cursor key for single-item result.

Per the JSON Data Nesting Convention in patterns.md (lines 623-630), single-item results from create/get/set operations should use a singular domain key (e.g., { cursor: {...} }), while collections should use plural keys (e.g., { cursors: [...] }).

The current implementation returns { cursors: [{ ... }] } which is technically a collection with one item, but semantically this is setting a single cursor.

♻️ Suggested change for consistency

       if (this.shouldOutputJson(flags)) {
         this.logJsonResult(
           {
-            cursors: [
-              {
-                clientId: this.realtimeClient!.auth.clientId,
-                connectionId: this.realtimeClient!.connection.id,
-                position: (
-                  cursorForOutput as { position: { x: number; y: number } }
-                ).position,
-                data:
-                  (cursorForOutput as { data?: Record<string, unknown> })
-                    .data ?? null,
-              },
-            ],
+            cursor: {
+              clientId: this.realtimeClient!.auth.clientId,
+              connectionId: this.realtimeClient!.connection.id,
+              position: (
+                cursorForOutput as { position: { x: number; y: number } }
+              ).position,
+              data:
+                (cursorForOutput as { data?: Record<string, unknown> })
+                  .data ?? null,
+            },
           },
           flags,
         );

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/spaces/cursors/set.ts` around lines 171 - 188, The JSON output
currently wraps the single set result in a plural array under cursors; change it
to emit a singular cursor object when only one item is returned: inside the
branch that calls this.logJsonResult (the block using
this.shouldOutputJson(flags)), construct and pass { cursor: { clientId:
this.realtimeClient!.auth.clientId, connectionId:
this.realtimeClient!.connection.id, position: (cursorForOutput as { position: {
x: number; y: number } }).position, data: (cursorForOutput as { data?:
Record<string, unknown> }).data ?? null } } instead of { cursors: [ ... ] };
keep the rest of the call (flags) unchanged and only adjust the top-level
key/name used for single-item responses.

src/commands/spaces/locations/get-all.ts (1)

79-82: Use this.logToStderr() for warning output.

Similar to the cursors/get-all command, warning messages should be written to stderr.

Suggested fix

        } else if (entries.length === 0) {
-          this.log(
-            formatWarning("No locations are currently set in this space."),
-          );
+          this.logToStderr(
+            formatWarning("No locations are currently set in this space."),
+          );
         } else {

Based on learnings: "Always use this.logToStderr(formatWarning('...')) for warning messages."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/spaces/locations/get-all.ts` around lines 79 - 82, Replace the
warning that currently uses this.log(formatWarning(...)) with
this.logToStderr(formatWarning(...)) in the branch that handles entries.length
=== 0; locate the else-if that checks entries.length === 0 (where
formatWarning("No locations are currently set in this space.") is used) and call
this.logToStderr with the same formatWarning output instead of this.log.

src/commands/spaces/cursors/get-all.ts (1)

71-72: Use this.logToStderr() for warning output.

Per project conventions, warning messages should be written to stderr rather than stdout to avoid polluting normal output.

Suggested fix

       } else if (cursors.length === 0) {
-        this.log(formatWarning("No active cursors found in space."));
+        this.logToStderr(formatWarning("No active cursors found in space."));
       } else {

Based on learnings: "In the ably/ably-cli TypeScript (oclif) codebase, do not use this.warn() directly in command implementations. Always use this.logToStderr(formatWarning('...')) for warning messages."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/spaces/cursors/get-all.ts` around lines 71 - 72, The warning is
currently being logged to stdout using this.log(formatWarning(...)) in the
get-all command; change it to write to stderr by replacing that call with
this.logToStderr(formatWarning("No active cursors found in space.")) so it
follows the project's convention for warnings in the get-all command
implementation (file: src/commands/spaces/cursors/get-all.ts, locate the branch
where cursors.length === 0 and update the this.log call).

test/unit/commands/spaces/locations/get-all.test.ts (1)

64-69: Assert the serialized coordinates here, not just the key.

toHaveProperty("location") still passes for null or the wrong payload, so this won’t catch regressions in the new map-to-array formatter.

🔎 Tighten the assertion

       expect(resultRecord!.locations[0]).toHaveProperty(
         "connectionId",
         "conn-1",
       );
-      expect(resultRecord!.locations[0]).toHaveProperty("location");
+      expect(resultRecord!.locations[0]).toHaveProperty("location", {
+        x: 100,
+        y: 200,
+      });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/unit/commands/spaces/locations/get-all.test.ts` around lines 64 - 69,
The test currently only asserts that resultRecord.locations[0] has a "location"
property which allows null/wrong values to slip through; update the assertion to
validate the serialized coordinates payload itself (for example assert
resultRecord!.locations[0].location equals the expected coordinates object/array
produced by the map-to-array formatter). Locate the assertion in the
get-all.test.ts where resultRecord!.locations[0] is checked and replace the
toHaveProperty("location") check with a strict equality/assertion against the
expected serialized coordinates (matching the output shape produced by the
map-to-array formatter).

test/unit/commands/spaces/locations/subscribe.test.ts (1)

59-73: Avoid wall-clock sleeps in these unit tests.

These 50ms waits make the assertions timing-dependent. On a busy runner the handler can still be unset, and on a fast exit the update can be emitted after runCommand() has already finished. It’s more deterministic to fire the update from the subscribe mock itself or coordinate with fake timers/a controllable promise.

Also applies to: 100-112

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/unit/commands/spaces/locations/subscribe.test.ts` around lines 59 - 73,
The test uses a 50ms setTimeout to wait for the subscribe handler to be
registered (locationHandler) which makes the test flaky; change the
space.locations.subscribe mockImplementation to synchronously emit the mock
update (i.e., call handler(mockUpdate) inside the mock) or return a controllable
promise that resolves when the handler is set and awaited by the test so
runCommand([... "spaces:locations:subscribe" ...]) sees the update
deterministically; update the logic around the locationHandler variable and the
subscribe mock so the test does not rely on wall-clock sleeps.

test/unit/commands/spaces/locks/subscribe.test.ts (2)

124-126: Use captureJsonLogs() instead of parseNdjsonLines() in this unit test

For mocked unit tests, prefer the repo-standard JSON capture helper to keep assertions consistent with other unit suites and reduce parser-coupling drift.

Suggested update

-import { parseNdjsonLines } from "../../../../helpers/ndjson.js";
+import { captureJsonLogs } from "../../../../helpers/ndjson.js";
...
-      const records = parseNdjsonLines(stdout);
+      const records = captureJsonLogs(stdout);

Based on learnings: In unit tests for this repo, JSON output assertions should use captureJsonLogs() from test/helpers/ndjson.ts rather than manual/parsing-oriented approaches.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/unit/commands/spaces/locks/subscribe.test.ts` around lines 124 - 126,
Replace the manual NDJSON parsing with the repo-standard JSON capture helper:
remove the use of parseNdjsonLines(stdout) in this test and instead call
captureJsonLogs() (imported from test/helpers/ndjson.ts) to obtain the logged
records (await if it returns a promise), then locate the event record the same
way (const eventRecord = records.find((r) => r.type === "event" && r.lock));
also update imports to remove parseNdjsonLines and add captureJsonLogs so
assertions remain consistent with other unit tests.

---

158-165: Tighten the rejection-path assertion

Consider asserting the error message (or command component) in addition to error being defined; this avoids passing on unrelated failures.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/unit/commands/spaces/locks/subscribe.test.ts` around lines 158 - 165,
The test currently only checks that an error was defined after calling
runCommand for the "spaces:locks:subscribe" command, which can mask unrelated
failures; update the assertion to inspect the error message or command-specific
component returned by runCommand (e.g., check error.message or error.output) to
ensure it contains a distinguishing token such as "spaces:locks:subscribe" or
the space name "test-space". Locate the failing test using runCommand in this
file and replace or add the loose expect(error).toBeDefined() with a tighter
check like expect(error.message).toContain('spaces:locks:subscribe') or
expect(error.message).toContain('test-space') so the test only passes for the
intended rejection path.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes